Skip to content

Fix #81206: Multiple PHP processes crash with JIT enabled #7208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 29, 2021

The fix for bug 80175 (PR #6268) broke most SAPIs wrt. JIT, most
notably cgi-fcgi, because for these SAPIs SUCCESSFULLY_REATTACHED
actually has to set the reattached flag.


I'm not sure whether some other APIs might need to be handled as well:

static const char *supported_sapis[] = {
"apache",
"fastcgi",
"cli-server",
"cgi-fcgi",
"fpm-fcgi",
"fpmi-fcgi",
"apache2handler",
"litespeed",
"uwsgi",
NULL
};

What is "uwsgi"?

The fix for bug 80175 (PR php#6268) broke most SAPIs wrt. JIT, most
notably cgi-fcgi, because for these SAPIs `SUCCESSFULLY_REATTACHED`
actually has to set the `reattached` flag.
@codecov-commenter

This comment has been minimized.

@nikic
Copy link
Member

nikic commented Jun 30, 2021

What's the relevant difference between apache2handler and other SAPIs here? Can you add a code comment for why it has special handling?

@cmb69
Copy link
Member Author

cmb69 commented Jul 3, 2021

Actually, that may only be relevant for mpm_winnt, where the control process attaches to SHM, and afterwards the handler process reattaches, although it needs to do the JIT initialization. I wonder if the proper solution would be to suppress OPcache initialization (or at least JIT initialization) for the control process in the first place. I have no idea how to do that, and what would need to be done for Linux, especially for worker and event (although these are not recommended according to the PHP manual).

We need to avoid resetting the JIT for all SAPIs, but we need to
initialize the JIT handlers even when only reattaching on Windows.
@cmb69
Copy link
Member Author

cmb69 commented Jul 4, 2021

Well, after more debugging I found that there is no relevant difference between the SAPIs here at all. The problem is just that we need to initialize the tracing JIT handlers for tracing even when reattaching on Windows. The new patch does this in the most simple way, namely by ignoring ZEND_VM_KIND_HYBRID which is not available on Windows anyway, and by doing the initialization on Windows only (I don't think this initialization is ever needed on other OSs).

Note that it was sufficient for the OP to just revert 2a545ba, because they had opcache.jit=1205 (= "function").

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, this makes sense. The stubs themselves are in SHM to which we reattach, but we need to reload the pointers to those stubs in per-process memory.

@dktapps
Copy link
Contributor

dktapps commented Jul 16, 2021

I don't think this initialization is ever needed on other OSs

Why is this? The handlers are defaulted to NULL, and only inited currently in zend_jit_make_stubs() - I don't see any other place where they are initialized, nor any special behaviour for *nix - am I missing something here?

@cmb69
Copy link
Member Author

cmb69 commented Jul 17, 2021

@dktapps, I think that re-attaching is a Windowes only thing.

@dktapps
Copy link
Contributor

dktapps commented Jul 17, 2021

I eventually managed to answer my own question - fork() is used everywhere except Windows, and SHM reattach is only done from forked subprocess on non-Windows. In a forked subprocess these handlers would already be initialized anyway because of the forking.

@cmb69 cmb69 closed this in ef77d3c Jul 19, 2021
@cmb69 cmb69 deleted the cmb/81206 branch July 19, 2021 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants